Skip to content

Feature/multi broker sync - #9

Merged
sahim99 merged 19 commits into
mainfrom
feature/multi-broker-sync
Sep 7, 2026
Merged

Feature/multi broker sync#9
sahim99 merged 19 commits into
mainfrom
feature/multi-broker-sync

Conversation

@Gyaan507

@Gyaan507 Gyaan507 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Gyaan507 and others added 18 commits September 6, 2026 15:33
Drop the preprod-only ci-image pin and disable Mongo auto-index creation in prod values so the /sync image can boot safely.

Co-authored-by: Cursor <cursoragent@cursor.com>
Match preprod and ignore helm/ci-image.yaml so feature pins cannot land on main.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 772a69d1-37fa-4d9c-9093-fba80d704d7d


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Gyaan507
Gyaan507 requested a deployment to preprod-am-document-processor September 7, 2026 09:30 — with GitHub Actions Waiting

@sahim99 sahim99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: 🔴 Requesting Changes

I have reviewed this PR and found a critical security issue that must be fixed before merging, along with a few other observations.

🔴 BLOCKER:

  • Authentication Bypass Risk: In DocumentProcessorController.java (resolveUserId), the PR removes the 401 Unauthorized exception and replaces it with a hardcoded fallback to "local-dev-user".
    If this ships to production, any unauthenticated request will successfully process documents and attach them to a shared local-dev-user account, bypassing security. You must restore the 401 Unauthorized throw. (If you need a local bypass for testing, wrap it in a @Profile("!prod & !preprod") check).

🟠 MAJOR:

  • Kafka Contract Risk: In MessagingEventService.sendBatchCompletedEvent, you are reusing PortfolioUpdateEvent but passing the batchId as the id field. Does downstream am-portfolio or am-analysis expect that id to strictly be a portfolioId? If downstream queries the database for a Portfolio using that UUID, it will fail. Please verify the Kafka contract.

🟡 MINOR / NIT:

  • Memory Pressure (OOM Risk): In DocumentProcessorService.submitBatchSync, you are eagerly calling entry.getFile().getBytes() to load files into memory. For typical 1-2MB PDFs, this is fine, but if a user uploads five 50MB files, that's 250MB loaded directly into heap per request. Ensure spring.servlet.multipart.max-file-size is strictly enforced.
  • Test Coverage: Consider adding unit tests for MultiPortfolioSplitterFactory and BrokerDetectionService to ensure the fallback logic works properly.

Restore 401 when unauthenticated outside local profiles, stop publishing batchId as PortfolioUpdateEvent id, and raise multipart request cap for 5-file sync.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Gyaan507
Gyaan507 requested a deployment to preprod-am-document-processor September 7, 2026 12:14 — with GitHub Actions Waiting
@Gyaan507
Gyaan507 requested a deployment to preprod-am-document-processor September 7, 2026 12:15 — with GitHub Actions Waiting
@sahim99
sahim99 merged commit 5865761 into main Sep 7, 2026
29 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants